Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throws errors when params would result in problems. #116

Merged
merged 1 commit into from
Oct 28, 2016

Conversation

nathanhammond
Copy link
Contributor

@nathanhammond nathanhammond commented Oct 4, 2016

route-recognizer forces both dynamic segments and glob segments to always be at least one character long at match time. This has been the case since the dawn of time.

Three lines below each of those in the generate function we find that there are no checks to prevent generation of a URL which route-recognizer is unable to recognize.

Proposed solution: we should throw an error during dynamic and glob route generation if the parameter is an empty string.

(Issue reported by @meirish.)

@nathanhammond nathanhammond changed the title Empty string globs generate and match correctly. Throw an error when generating an unrecognizable route. Oct 4, 2016
@nathanhammond nathanhammond force-pushed the empty-string-globs branch 2 times, most recently from 66c1967 to b10bc46 Compare October 5, 2016 00:19
@nathanhammond
Copy link
Contributor Author

nathanhammond commented Oct 5, 2016

While in here should we also address these problems?

test("Fails reasonably when no parameters passed to dynamic segment", function() {
  var router = new RouteRecognizer();
  router.add([{"path":"/posts","handler":"posts"},{"path":"/:secret/edit","handler":"edit"}], { as: "edit"})

  // Expects a second argument.
  router.generate("edit"); // => error, undefined is not an object

  // Expects a parameter of a particular name.
  router.generate("edit", {}); // => /posts/undefined
});

@rwjblue
Copy link
Collaborator

rwjblue commented Oct 14, 2016

@nathanhammond - This seems reasonable to me, but we may want to include more information in the error message (handler.toString() or something?). Also, what happens in Ember today if you use an empty string for a glob/dynamic segment?

@nathanhammond
Copy link
Contributor Author

@rwjblue: What happens today: it happily generates a route when you pass an empty string and you can link-to to it, but blows up on refresh or open in new tab.

The latter one in my first comment can simply provide a better message for these scenarios.

@nathanhammond nathanhammond force-pushed the empty-string-globs branch 2 times, most recently from 4010f01 to 0e48c2e Compare October 14, 2016 06:12
@nathanhammond nathanhammond changed the title Throw an error when generating an unrecognizable route. Throws errors when params would result in problems. Oct 14, 2016
@nathanhammond
Copy link
Contributor Author

nathanhammond commented Oct 14, 2016

This is complete. Assuming no further comments I intend to land it sometime this weekend. Review now! /cc @wycats @krisselden @mixonic

(Note: the messages are as good as they reasonably need to be, the stack trace will make it plenty clear what blew up.)

(Note: landing doesn't mean an immediate release, I intend to make sure that this goes the full span of an Ember canary to get as much feedback as possible on how often people end up broken.)

@nathanhammond
Copy link
Contributor Author

Given no additional feedback for two weeks, I'm landing this.

@nathanhammond nathanhammond merged commit dcc3845 into tildeio:master Oct 28, 2016
@nathanhammond nathanhammond deleted the empty-string-globs branch October 28, 2016 23:03
nathanhammond added a commit to nathanhammond/ember-route-recognizer that referenced this pull request Nov 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants